Skip to content

Group friendly fire: warn on Harmony conflicts (4/4, part of #277) - #304

Merged
Zaldaryon merged 3 commits into
feat/issue-277-group-friendly-firefrom
feat/issue-277-ff-harmony-watchlist
Sep 9, 2026
Merged

Zaldaryon merged 3 commits into
feat/issue-277-group-friendly-firefrom
feat/issue-277-ff-harmony-watchlist

Conversation

@Zaldaryon

@Zaldaryon Zaldaryon commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fourth of four follow-ups to #300. Adds a targeted Harmony-conflict warning for the group friendly-fire toggle and checks both the boot and runtime configuration paths.

Base branch is feat/issue-277-group-friendly-fire (#300). This PR is independent of the documentation and source-seam follow-ups, but its watchlist covers only methods present in this base. Rebase onto indev after #300.

Change

StratumHarmonyVisibility already logs every mod Harmony patch at boot for triage. This adds WarnFriendlyFireConflicts: when /friendlyfire off is active, it walks the patched methods used by the toggle and logs a warning naming any mod that also patches one. The watchlist covers Entity.ReceiveDamage / ShouldReceiveDamage, EntityAgent.OnInteract, EntityBehaviorHealth.OnEntityReceiveDamage, ServerSystemEntitySimulation.HandleEntityInteraction, and EntityProjectileBase.CanDealDamage / DealDamage / ImpactOnEntity.

  • sources/VintagestoryLib/Vintagestory.Server/StratumHarmonyVisibility.cs: owns the watchlist and the walk. ServerMain.CreateExplosion is not listed because explosions are not part of Group friendly fire: /friendlyfire toggle (part of #277) #300's base.
  • sources/VintagestoryLib/Vintagestory.Server/ServerSystemStratum.cs: runs the warning after mod loading at OnBeginRunGame.
  • sources/VintagestoryLib/Vintagestory.Server/StratumFriendlyFireSystem.cs: runs the warning after startup, /friendlyfire, and /stratum reload apply the current state.

It runs regardless of Diagnostics.LogModHarmonyPatches and stays silent when the toggle is on or nothing patches the watched methods.

Why only a warning

There is no Harmony-safe seam. A prefix that returns false and applies damage itself defeats any single check. The friendly-fire stack enforces the rule at independent points, and this warning makes a conflicting patch visible instead of silent.

Gates

  • Two-pass build green, 4 pre-existing NU1904 warnings.
  • bash scripts/smoke-test.sh: PASS: server reached RunGame, no fatal errors, 13 console command(s) verified. The boot scan runs with the default toggle on and produces no warning without conflicting mods.
  • scripts/extract-patches.sh: no patch or vanilla-source change. Both files are in sources/.

Type

  • Bug fix
  • Performance
  • New feature
  • Refactor or cleanup
  • Docs or build

Checklist

  • scripts/extract-patches.sh ran clean (no patch touched).
  • dotnet build VintageStory.slnx -c Release -p:EmbedPatchedFiles=true is green.
  • Every vanilla edit has a // Stratum marker (none in this PR).
  • No vanilla source committed.
  • Tested on a real server start, not just compilation.

Related issues

Part of #277

StratumHarmonyVisibility already logs every mod Harmony patch for
triage. This adds a targeted check: when /friendlyfire is off, walk the
patched methods for the ones the toggle depends on (the damage entry
points, the melee interaction handler, the projectile impact path,
CreateExplosion) and log a warning naming any mod that also patches one.

A mod that replaces one of those and does not call the original can
silently defeat the toggle. Nothing can prevent that, so the goal is
just to make it visible instead of a mystery. Runs regardless of the
Diagnostics.LogModHarmonyPatches flag, since it is one short feature
warning rather than the full patch dump, and stays silent when the
toggle is on or nothing patches those methods.

Part of #277

@Pixnop Pixnop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice reuse of the existing StratumHarmonyVisibility walk rather than a new scanner, and CollectOwners mirrors CountOwners cleanly. One additive commit, sources only, compiles against #300 alone.

One thing I think bites in practice. The guard at StratumHarmonyVisibility.cs:99 only walks when BlockGroupDamage is already true, and the single call site is OnBeginRunGame (ServerSystemStratum.cs:109). AllowGroupDamage defaults to true, so on a stock server the method returns at line 102 and nothing is checked, and an admin who later runs /friendlyfire off gets the protection but never the warning, which is the case it exists for. It also means the smoke run in the body only exercised the early return. Calling it from StratumFriendlyFireSystem.Apply would cover boot, the command and /stratum reload in one place.

Two smaller ones. Line 87 watches ServerMain.CreateExplosion and lines 88-91 the interaction and projectile seams, which only become Stratum seams once #302 and #303 land; on this base the entry asks an admin to verify a block that never happens. And line 123 removes "(unknown)" right after line 142 produced it, so an unattributed patch is dropped, and when it is the only one on a watched method the warning is skipped outright. Keeping "(unknown)" in the message is strictly more useful.

Line 74 says "when the toggle is on we name the mod", line 95 says the opposite. The code warns when friendly fire is off. Which wording do you want to keep?

@Pixnop Pixnop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to a change request: as written the walk never runs on a default server and never re-runs on /friendlyfire off, so the warning cannot reach the case it was written for. Calling it from StratumFriendlyFireSystem.Apply covers boot, the command and reload. The watchlist entries and the (unknown) owner are secondary.

@Zaldaryon
Zaldaryon requested a review from Pixnop September 8, 2026 22:36
@Zaldaryon

Copy link
Copy Markdown
Contributor Author

Re-review requested

The requested changes are pushed and verified. Please re-review the current head before merge.

@Zaldaryon
Zaldaryon requested review from Pixnop and removed request for Pixnop September 8, 2026 22:48

@Pixnop Pixnop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, two of the three are settled. StratumHarmonyVisibility.cs:74 now matches the guard at 99, and dropping the Remove call means an unattributed patch survives the count check at 123 and reaches the message at 128.

The call site move is only half of it. Apply at StratumFriendlyFireSystem.cs:45 covers /friendlyfire (104) and /stratum reload (CmdStratum.cs:216), both after mods are loaded. Boot went the other way. The constructor's Apply at 27 runs from ServerSystemCommands.OnBeginConfiguration, which ServerMain calls at 1337, and mods install their Harmony patches later: OnLoadAssets at 1340 runs the Pre and Start phases, OnBeginModsAndConfigReady at 1346 runs StartServerSide. So GetAllPatchedMethods is empty when the boot walk runs, and a server starting with AllowGroupDamage false plus a conflicting mod warns nothing, which the OnBeginRunGame call you removed did catch. Keeping both call sites covers all three paths.

Still open from the first pass: ServerMain.CreateExplosion at 87, while StratumFriendlyFireConfig.cs:9 on this base says explosions are not covered by the toggle. And the body still describes the OnBeginRunGame call and the smoke run that went through it, so it needs a refresh whichever way the call site lands.

@Zaldaryon
Zaldaryon requested a review from Pixnop September 9, 2026 00:00
@Zaldaryon

Copy link
Copy Markdown
Contributor Author

Re-review requested

The Harmony warning now runs at boot after mod loading and after each StratumFriendlyFireSystem.Apply. The watchlist no longer includes ServerMain.CreateExplosion, which is not part of the #300 base.

Latest commit: 2a00622.

Verification: make build passed with 0 errors and 4 solution warnings. bash scripts/smoke-test.sh passed with 13 console commands and reached WorldReady. Please re-review the current head.

@Pixnop Pixnop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That closes it. ServerSystemStratum.cs:109 runs WarnFriendlyFireConflicts from OnBeginRunGame again, after OnLoadAssets and StartServerSide have let mods patch, and Apply at StratumFriendlyFireSystem.cs:45 still covers /friendlyfire and /stratum reload, so all three paths see the real patch list. CreateExplosion is out of the watchlist for this base, and the body describes the two call sites as they are. Approving.

@Zaldaryon
Zaldaryon merged commit 45f3556 into feat/issue-277-group-friendly-fire Sep 9, 2026
1 check passed
@Zaldaryon
Zaldaryon deleted the feat/issue-277-ff-harmony-watchlist branch September 11, 2026 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants